Add overridable wrapper to CreatibutorsModal role SelectField #1759
Conversation
|
I hope this overriadable ID |
|
Hi @kpsherva could you please look into this? Would it be possible to merge this or is something missing? |
|
could you remove the merge commits and push again? steps:
|
SarahW91
force-pushed
the
creatibutor-modal-overridable
branch
from
4d5e9d9
to
d91abc8
Compare
I did and I hope this works for you now, @carlinmack. If it does, could you please merge now? |
SarahW91
force-pushed
the
creatibutor-modal-overridable
branch
from
d91abc8
to
ecbd94b
Compare
| </Button> | ||
| } | ||
| /> | ||
| <OverridableContext.Provider value={overriddenComponents}> |
There was a problem hiding this comment.
By putting the context provider here, we are risking having many nested context providers and it might have unpredicted consequences (one of them being confusion). Why not having the context provider at the level of the form rendering (RDMDepositForm in invenio-app-rdm?)
There was a problem hiding this comment.
@mkloeppe Could you look into this?
| optimized | ||
| scrolling | ||
| /> | ||
| <Overridable id="InvenioRdmRecords.Deposit.CreatibutorsModalRoleSelectField.Container"> |
There was a problem hiding this comment.
you are missing passing props to this component. Without props passed to overridable you will not be able to access any props in your overridden cmp (fe. you don't want to override fieldPath, because the form will not work)
There was a problem hiding this comment.
@mkloeppe Could you look into this?
❤️ Thank you for your contribution!
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: